Conversation
auth token now respects --output text when explicitly set, outputting just the access token string suitable for piping. JSON remains the default for backward compatibility. Also returns write errors instead of discarding them. Co-authored-by: Isaac
simonfaltum
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: low Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @andrewnester, @anton-107, @denik, @shreyas-goenka Suggestions based on git history of 2 changed files (2 scored). See CODEOWNERS for path-specific ownership rules. |
| wantSubstr string | ||
| wantJSON bool |
There was a problem hiding this comment.
Could we compare the whole output instead? This will require a single string no matter if the output is in text or JSON. Default JSON marshaling is deterministic so the output should be stable.
| return cmd | ||
| } | ||
|
|
||
| func writeTokenOutput(cmd *cobra.Command, t *oauth2.Token) error { |
There was a problem hiding this comment.
Passing cmd couples this simple function to Cobra. Could we rather take a boolean isOutput or an enum as parameter? This would make the function trivial to test on its own without the need for any Cobra machinery.
| return "<error>", nil | ||
| } | ||
|
|
||
| func TestTokenCommand_TextOutput(t *testing.T) { |
There was a problem hiding this comment.
This feels more complicated than it needs to be. Should we rather test the new private function you've added?
I'm fine with testing the whole command but then I think we should refactor it so that it allows easy injection of its dependencies (e.g. the token minting part).
renaudhartert-db
left a comment
renaudhartert-db
left a comment
There was a problem hiding this comment.
LGTM assuming resolution of the open comments.
2 participants
Why
auth tokenalways outputs JSON, ignoring the--outputflag. If you want to pipe just the token string into another tool, you have to parse JSON. That's unnecessary friction for a common scripting pattern.Changes
auth tokennow respects--output textwhen explicitly set, outputting just the access token string (with a trailing newline) suitable for piping. JSON remains the default for backward compatibility.Only honors the explicit
--output textflag, not implicit text mode (e.g. fromDATABRICKS_OUTPUT_FORMAT), so existing scripts that parse JSON output won't break.Also fixes discarded write errors (
_, _replaced with proper error returns).Test plan
--output json, and--output textmodescmd/authtests pass